Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjustments to test with python 3.12 #122

Merged
merged 2 commits into from
May 24, 2024
Merged

Adjustments to test with python 3.12 #122

merged 2 commits into from
May 24, 2024

Conversation

addyess
Copy link
Contributor

@addyess addyess commented May 24, 2024 •
edited
Loading

While testing with python3.12, i had to bump pyfakefs==5.5.0 due to some changes in pathlib

After this adjustment, i found bad tests in test_sysctl.

instead of

assert my_mock.called_with(...)

swapped to

my_mock.assert_called_once_with(...)

Also, it seems codespell now doesn't like assertIn so let's start ignoring reasonable words

@addyess
Copy link
Contributor Author

addyess commented May 24, 2024

@benhoyt I happened to find these unit tests failures when running the unit tests on python 3.12

dimaqq
dimaqq approved these changes May 24, 2024
View reviewed changes
Copy link

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on pyfakefs update. I vaguely remember having to do that in the past in some other repo.

tests/unit/test_sysctl.py
Comment on lines +211 to +213
mock_output.assert_called_once_with(
["sysctl", "-n", "vm.swappiness", "other_value"], stderr=-2, universal_newlines=True
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old test was just bad, wasn't it?
Meaning that the 2 settings were fetched serially before, and in a single command now. The code change must've been done without correcting the tests at some point prior, mustn't it?

I wonder if stderr=-2 is actually significant, but then while the value could be checked against ANY, the presence of the kwarg cannot be easily ignored, which is a bit of an impasse.

Happy to have this change regardless :)

@addyess
Copy link
Contributor Author

addyess commented May 24, 2024

@dimaqq i don't have a clue why this integration test is failing. It's failing in my other PR (#121) as well

benhoyt
benhoyt approved these changes May 24, 2024
View reviewed changes
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these changes. Mind taking a look the failing integration test, too?

@dimaqq
Copy link

dimaqq commented May 24, 2024

charms.operator_libs_linux.v2.snap.SnapError: Snap: 'lxd'; command ['snap', 'refresh', 'lxd', '--channel="latest"'] failed with output = '2024-05-24T03:43:44Z INFO Waiting for "snap.lxd.daemon.service" to stop.\n'

Perhaps the old version needs more time to stop?
Or maybe we're not reading the entire output?

@addyess
Copy link
Contributor Author

addyess commented May 24, 2024

Thanks for these changes. Mind taking a look the failing integration test, too?

Aye:

looks like refreshing the lxd snap fails if we're in a big hurry?

2024-05-24T03:43:44Z INFO Waiting for "snap.lxd.daemon.service" to stop.\n

@addyess
Copy link
Contributor Author

addyess commented May 24, 2024

there's also this in the pytest logs:

tests/integration/test_snap.py::test_snap_set_and_get_with_typed 
-------------------------------- live log call ---------------------------------
INFO     charms.operator_libs_linux.v2.snap:snap.py:589 Refreshing snap lxd, revision None, tracking latest
error: cannot perform the following tasks:
- Run configure hook of "lxd" snap if present (run hook "configure": error: cannot communicate with server: Post "http://localhost/v2/snapctl": dial unix /run/snapd-snap.socket: connect: no such file or directory)
FAILED

looks like something has gone wrong with snapd on the machine during refresh that it cannot communicate with lxd??

@benhoyt
Copy link
Collaborator

benhoyt commented May 24, 2024

Ah yes, I just noticed that too @addyess. Weird, it's like the snap daemon isn't running or something. We'll let you off the hook. ;-) I'll merge this PR and we can address the integration test failures separately.

@benhoyt benhoyt merged commit 30e4f75 into canonical:main May 24, 2024
5 of 6 checks passed
@benhoyt
Copy link
Collaborator

benhoyt commented May 24, 2024

Opened #123 to track fixing the integration test.

@addyess addyess deleted the akd/testing-with-py-3.12 branch May 24, 2024 13:05
@addyess
Copy link
Contributor Author

addyess commented May 24, 2024

We'll let you off the hook. ;-)

This...time...only...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimaqq dimaqq dimaqq approved these changes

@benhoyt benhoyt benhoyt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@addyess @dimaqq @benhoyt